[wasm] Use instantiating stubs for default interface dispatch - #133924
davidwrighton wants to merge 1 commit into
Conversation
Fixes dotnet#133466 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/crossgen-contrib |
There was a problem hiding this comment.
🔵 Needs a closer look
The Wasm ReadyToRun dispatch change warrants final human review.
Pull request overview
This PR fixes Wasm ReadyToRun default interface dispatch and re-enables the affected regression test.
Changes:
- Select instantiating stubs for applicable Wasm interface dispatches.
- Remove the browser ReadyToRun test skip.
File summaries
| File | Summary |
|---|---|
src/tests/Loader/classloader/StaticVirtualMethods/InterfaceVariance/ComplexHierarchyPositive.cs |
Re-enables browser ReadyToRun coverage. |
src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs |
Adds instantiating-stub selection for Wasm default interface dispatch. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
| if (!targetMethod.HasInstantiation) | ||
| { | ||
| // If it is also a default interface method call, it should go through instantiating stub. | ||
| useInstantiatingStub = useInstantiatingStub || (targetMethod.OwningType.IsInterface && !originalMethod.IsAbstract); |
There was a problem hiding this comment.
Is there a particular reason why we need instantiating stubs on wasm ? They are always in danger of being interpreted and I'm wondering why would they even be needed. Isn't any method able to obtain its method context directly from the PEP.
There was a problem hiding this comment.
Our shared generics design currently requires the distinction, since shared generic code dispatches to other shared generic code, assuming that the generic dictionary only carries the generic context argument in it. As you note, in principle this isn't needed, since the PEP could represent that context, and we could dispatch to code pointers held in the generic dictionary instead of just getting the generic context from there. However, that would make WASM an even weirder platform than it already is, so I'd like to avoid making such a change if we can.
Summary
CORINFO_VIRTUALCALL_LDVIRTFTNpathComplexHierarchyPositiveThe instantiating stub keeps the interface dispatch call site on its normal ABI while supplying the generic interface context if dispatch resolves to the shared default implementation.
Testing
./build.sh clr.tools -c checkedsrc/tests/build.sh -browser -checked -test:Loader/classloader/StaticVirtualMethods/InterfaceVariance/ComplexHierarchyPositive.csprojComplexHierarchyPositivepassed with exit code 100Fixes #133466
Note
This pull request description was generated with GitHub Copilot.